UEFI: Add support for different framebuffer configs on real hardware vs VMs by kennystrawnmusic · Pull Request #364 · rust-osdev/bootloader

kennystrawnmusic · 2023-04-15T19:08:24Z

It may look nice to have a slighltly lower resolution when virtualizing the OS you're building, but during real hardware tests it makes far more sense to choose a higher-resolution mode than during QEMU tests. Otherwise, you're left with 2 options:

Configure the bootloader to always use a higher resolution, making QEMU hog your entire screen
Configure the bootloader to use the default (lower) resolution resulting in all the text looking ugly on real hardware

So, creating this pull request to fix the problem. Using raw_cpuid makes it possible to check whether or not you're running inside a hypervisor before setting a display mode, which is important for improving this, and using .max() on the mode iterator when real hardware is detected ensures that the absolute highest resolution possible is used in that case.

…lint

Co-authored-by: Philipp Oppermann <dev@phil-opp.com>

uefi/src/main.rs

phil-opp · 2023-04-16T10:44:02Z

Thanks a lot for this PR!

I agree that the current framebuffer configuration is missing some crucial outputs. We currently choose the last mode that matches the min_width and min_height requirements, but I don't think that UEFI guarantees any particular ordering of GOP modes. So it depends on the machine's firmware whether we end up with the smallest or largest mode.

So I think that we're missing at least the following config options:

Maximum bounds for the resolution.

I imagine that this this would be useful when e.g. a 1080p monitor is connected to a 4k-capable GPU?

A way to specify whether we prefer the highest or lowest resolution within the min/max bounds.
A way to special-case the framebuffer configuration when using QEMU or other virtual machines.

This PR seems to partially address points 2 and 3, but it doesn't allow configuring the new behavior. I think I would prefer a less-opinionated approach that allows users to configure the behavior themselves. So how about the following:

We add new maximum_framebuffer_height and maximum_framebuffer_width fields to bootloader_boot_config::FrameBuffer.
We add a new field to control whether to prefer the highest or lowest available resolution in the given min/max range. For example, this could be a prefer_resolution field with Highest and Lowest as options, or a boolean flag named e.g. prefer_lowest_resolution.
We add a new frame_buffer_vm: Framebuffer field to bootloader::BootConfig. The bootloader will use this configuration when it detects that it's running inside a virtual machine (as implemented in this PR).
We set reasonable defaults for the new fields. For example:
- frame_buffer_vm.prefer_lowest_resolution = true, and probably some min bounds to avoid a tiny window
- frame_buffer.prefer_lowest_resolution = false and no maximum bounds
We update both the BIOS and UEFI implementations to respect the configuration.

This way, we get a similar behavior by default, but users can adjust the behavior as they like. What do you think?

uefi/src/main.rs

kennystrawnmusic · 2023-04-30T23:12:55Z

We add a new frame_buffer_vm: Framebuffer field to bootloader::BootConfig. The bootloader will use this configuration when it detects that it's running inside a virtual machine (as implemented in this PR).

frame_buffer_vm: Option<FrameBuffer> would be an even better idea still than this. After all, the current implementation already has it as an optional type as is.

Back to the drawing board; going to reconfigure this using that suggestion. Much better I think.

kennystrawnmusic · 2023-04-30T23:37:55Z

Alright, submitted newer commits that address both of your concerns:

@bjorn3: now using HypervisorInfo::identify() to single out Xen as a special case deserving of treatment like physical hardware
@phil-opp: added new configuration options instead of defaulting to resolutions that may be too high for some people's use cases.

kennystrawnmusic · 2023-05-05T18:31:57Z

Thanks a lot for this PR!

I agree that the current framebuffer configuration is missing some crucial outputs. We currently choose the last mode that matches the min_width and min_height requirements, but I don't think that UEFI guarantees any particular ordering of GOP modes. So it depends on the machine's firmware whether we end up with the smallest or largest mode.

So I think that we're missing at least the following config options:

Maximum bounds for the resolution.

I imagine that this this would be useful when e.g. a 1080p monitor is connected to a 4k-capable GPU?

A way to specify whether we prefer the highest or lowest resolution within the min/max bounds.

A way to special-case the framebuffer configuration when using QEMU or other virtual machines.

This PR seems to partially address points 2 and 3, but it doesn't allow configuring the new behavior. I think I would prefer a less-opinionated approach that allows users to configure the behavior themselves. So how about the following:

We add new maximum_framebuffer_height and maximum_framebuffer_width fields to bootloader_boot_config::FrameBuffer.

We add a new field to control whether to prefer the highest or lowest available resolution in the given min/max range. For example, this could be a prefer_resolution field with Highest and Lowest as options, or a boolean flag named e.g. prefer_lowest_resolution.

We add a new frame_buffer_vm: Framebuffer field to bootloader::BootConfig. The bootloader will use this configuration when it detects that it's running inside a virtual machine (as implemented in this PR).

We set reasonable defaults for the new fields. For example:

frame_buffer_vm.prefer_lowest_resolution = true, and probably some min bounds to avoid a tiny window

frame_buffer.prefer_lowest_resolution = false and no maximum bounds

We update both the BIOS and UEFI implementations to respect the configuration.

This way, we get a similar behavior by default, but users can adjust the behavior as they like. What do you think?

Changed the title of this pull request to reflect these suggestions

polarathene · 2023-07-26T04:24:26Z

uefi/src/main.rs

+        if let Some(hypervisor) = CpuId::new().get_hypervisor_info() {
+            if let Hypervisor::Xen = hypervisor.identify() {
+                // Use same rules as real hardware since Xen uses the whole screen


I'm a bit puzzled about what this PR is doing.

I often use VMWare and QEMU/KVM for running guest VMs as I would a host, especially when enabling paravirtualization features for performance similar to host system. These guests may run windowed or fullscreen.

It's not too concerning for me as this is only applicable to a bootloader based on this project, but the motivation seems focused on QEMU windowed for development/testing purposes?

As a user, if I have a VM guest fullscreen on a display(s), I'd find this different implicit behaviour a bit confusing, especially since there's an exception made for Xen.

I don't think you find GRUB, rEFInd, or systemd-boot handling resolution used differently? I believe they have a config setting if you want to explicitly prefer a resolution or scaling/fit?

I'm not that familiar with the project, but is this just a default with a way to opt-out? Is there actual value in a physical + virtual framebuffer structs that this PR introduces for this distinction?

Or would it be better to match what other bootloader/managers do, and just provide a default with build (or runtime) config to have the scaling behaviour you prefer?

You could then more easily build for your VM testing, with a different config for production builds on real hardware (or VM guests). The detection to switch based on environment if desired beyond testing may be better served as an opt-in feature/config?

If you disagree, no worries 👍

Maybe consider documenting the behaviour then so it's easier to troubleshoot when a dev/user encounters it and tries to understand why it scales differently on most hypervisors.

…ng on UEFI

Test pull request to see if this works in my kernel

polarathene · 2023-10-02T22:47:17Z

@kennystrawnmusic what exactly happened?

I see you added new commits yesterday, then closed this PR with no context? (and my comment was entirely ignored too)

Looks like some of the PR was moved to a new PR: #394

kennystrawnmusic · 2023-10-03T11:22:07Z

@kennystrawnmusic what exactly happened?

I see you added new commits yesterday, then closed this PR with no context? (and my comment was entirely ignored too)

Looks like some of the PR was moved to a new PR: #394

The other PR was on a completely different branch that doesn't have these changes (only the ones that add in the SystemTable<Runtime> address while leaving the config structures unchanged) due to it being of a higher priority. Will reopen this one when I have the time to fix these checks (eventual intention is to reverse them, i.e. to only check for QEMU and nothing else, when determining what mode to use).

polarathene · 2023-10-03T11:56:07Z

I'd still like to discourage enforcing such behaviour on QEMU.

If the firmware is deployed to a VM it can be for production usage and utilize the full display. One would expect the resolution to not be messed with due to serving as a convenience during development when a hypervisor detected, it should have an opt-in / opt-out.

kennystrawnmusic and others added 10 commits March 27, 2023 10:54

Refactor UEFI implementation for compliance with rust-lang/rust#107457 …

a2fc6ce

…lint

rustfmt

fd818fc

rust-osdev#360: Resolve @phil-opp's concerns (1/5)

57bd85d

rust-osdev#360: Resolve @phil-opp's concerns (2/5)

ec7cbaa

rust-osdev#360: Resolve @phil-opp's concerns (3/5 + 4/5)

d18076e

Update tests/runner/src/lib.rs

5d153e3

Co-authored-by: Philipp Oppermann <dev@phil-opp.com>

Fix unused import warning

02ecbb2

Merge branch 'rust-osdev:main' into master

2aa4abf

Use highest resolution available on real hardware

84d3f62

rustfmt

d8d4b9c

kennystrawnmusic changed the title ~~UEFI: Configuring modes only makes sense inside QEMU, not on real hardware~~ UEFI: Configuring display modes manuall only makes sense inside QEMU, not on real hardware Apr 15, 2023

kennystrawnmusic changed the title ~~UEFI: Configuring display modes manuall only makes sense inside QEMU, not on real hardware~~ UEFI: Configuring display modes manually only makes sense inside QEMU, not on real hardware Apr 15, 2023

bjorn3 reviewed Apr 15, 2023

View reviewed changes

uefi/src/main.rs Outdated Show resolved Hide resolved

phil-opp reviewed Apr 16, 2023

View reviewed changes

uefi/src/main.rs Outdated Show resolved Hide resolved

kennystrawnmusic added 3 commits April 30, 2023 16:14

Merge branch 'rust-osdev:main' into master

ccb8bb2

rust-osdev#364: Use different configs for VMs vs real hardware

16330cd

rust-osdev#364: rustfmt

2f66e58

kennystrawnmusic added a commit to kennystrawnmusic/cryptos that referenced this pull request Apr 30, 2023

Port runner to new iteration of rust-osdev/bootloader#364 changes

fb1dffb

kennystrawnmusic changed the title ~~UEFI: Configuring display modes manually only makes sense inside QEMU, not on real hardware~~ UEFI: Add support for different framebuffer configs on real hardware vs VMs May 5, 2023

polarathene reviewed Jul 26, 2023

View reviewed changes

kennystrawnmusic added 6 commits September 6, 2023 13:57

Merge branch 'rust-osdev:main' into master

fb182e7

Pass the UEFI runtime services table address to the BootInfo if runni…

299a670

…ng on UEFI

Pass the UEFI runtime services table address to the BootInfo if runni…

68a123e

…ng on UEFI

rustfmt

c5b6e04

Merge remote-tracking branch 'origin/patch-1'

31f77fa

Test pull request to see if this works in my kernel

Remove redundant dependencies

0b028db

kennystrawnmusic added 4 commits October 1, 2023 17:55

Add back the Copy and Clone impls

35bcae6

Fix doc comments

5578f36

rustfmt

20dbee7

Merge commits pushed to pull request

82ea900

kennystrawnmusic closed this Oct 2, 2023

kennystrawnmusic added a commit to kennystrawnmusic/bootloader that referenced this pull request Oct 22, 2023

Undo changes in pull request rust-osdev#364

e531ee0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UEFI: Add support for different framebuffer configs on real hardware vs VMs#364

UEFI: Add support for different framebuffer configs on real hardware vs VMs#364
kennystrawnmusic wants to merge 23 commits intorust-osdev:mainfrom
kennystrawnmusic:master

kennystrawnmusic commented Apr 15, 2023 •

edited

Loading

Uh oh!

Uh oh!

phil-opp commented Apr 16, 2023

Uh oh!

Uh oh!

kennystrawnmusic commented Apr 30, 2023

Uh oh!

kennystrawnmusic commented Apr 30, 2023 •

edited

Loading

Uh oh!

kennystrawnmusic commented May 5, 2023

Uh oh!

polarathene Jul 26, 2023

Uh oh!

polarathene commented Oct 2, 2023 •

edited

Loading

Uh oh!

kennystrawnmusic commented Oct 3, 2023 •

edited

Loading

Uh oh!

polarathene commented Oct 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

kennystrawnmusic commented Apr 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

phil-opp commented Apr 16, 2023

Uh oh!

Uh oh!

kennystrawnmusic commented Apr 30, 2023

Uh oh!

kennystrawnmusic commented Apr 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kennystrawnmusic commented May 5, 2023

Uh oh!

polarathene Jul 26, 2023

Choose a reason for hiding this comment

Uh oh!

polarathene commented Oct 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kennystrawnmusic commented Oct 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

polarathene commented Oct 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kennystrawnmusic commented Apr 15, 2023 •

edited

Loading

kennystrawnmusic commented Apr 30, 2023 •

edited

Loading

polarathene commented Oct 2, 2023 •

edited

Loading

kennystrawnmusic commented Oct 3, 2023 •

edited

Loading